Skip to content

fix(pegboard-runner): clear terminal tunnel routes#4621

Merged
MasterPtato merged 3 commits intomainfrom
04-12-fix_pegboard-runner_clear_terminal_tunnel_routes
Apr 13, 2026
Merged

fix(pegboard-runner): clear terminal tunnel routes#4621
MasterPtato merged 3 commits intomainfrom
04-12-fix_pegboard-runner_clear_terminal_tunnel_routes

Conversation

@NathanFlurry
Copy link
Copy Markdown
Member

Description

Please include a summary of the changes and the related issue. Please also include relevant motivation and context.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@claude
Copy link
Copy Markdown

claude bot commented Apr 13, 2026

PR Review: Fix(pegboard-runner): Clear terminal tunnel routes

Overall: This is a clean, correct fix. Tunnel routes are now properly cleaned up after terminal messages, preventing stale entries from lingering in authorized_tunnel_routes.

What's Good

  • Correct ordering: Routes are cleared after the message is processed and forwarded. Clearing before would mean a processing failure could permanently lose the route.
  • Protocol parity: should_clear_tunnel_route_mk1 and should_clear_tunnel_route_mk2 are symmetric, consistent with the codebase engine runner parity requirement.
  • Clean refactor: Extracting route and clear_route before msg is consumed by the publish call is the right approach.
  • Test update: Switching the republishes_issued_* tests to use WebSocket messages (which do not clear routes) and asserting the route still exists is a good correctness check.

Issues

Missing tests for the clearing behavior - The PR adds several helper functions (response_start_message_mk2, response_start_message_mk2_with_stream, response_chunk_message_mk2, etc.) but they appear unused. No tests verify that routes are actually removed when terminal messages are sent. Consider adding tests for each clearing case:

  • ToServerResponseStart with stream: false -> route gone
  • ToServerResponseStart with stream: true -> route still present
  • ToServerResponseChunk with finish: true -> route gone
  • ToServerResponseChunk with finish: false -> route still present
  • ToServerResponseAbort -> route gone
  • ToServerWebSocketClose -> route gone

Unused helper functions will generate dead-code warnings - response_start_message_mk2, response_start_message_mk2_with_stream, response_chunk_message_mk2, response_start_message_mk1, response_start_message_mk1_with_stream, and response_chunk_message_mk1 are defined but not used in the tests shown. Either write the tests that use them or remove them.

Minor

The _ => false default in both should_clear_tunnel_route functions is reasonable. A brief comment explaining the intent (non-terminal message kinds keep the route open) would help future protocol additions understand the default behavior.

No Security or Correctness Concerns

The remove_async call on a missing key is a no-op for scc::HashMap, so there is no panic risk if a route was already removed. Route leaks on processing errors are acceptable since an error would propagate and tear down the connection, dropping the entire map.

@NathanFlurry NathanFlurry force-pushed the 04-12-fix_pegboard-runner_clear_terminal_tunnel_routes branch from d96b76e to c8659a3 Compare April 13, 2026 02:28
@NathanFlurry NathanFlurry force-pushed the 04-12-fix_pegboard-runner_clear_terminal_tunnel_routes branch 2 times, most recently from eeba1f7 to d0fd3ff Compare April 13, 2026 05:50
@MasterPtato MasterPtato force-pushed the 04-12-fix_pegboard_skip_protocol_version_keys_in_runner_pool_backfill branch from 0aff89a to f99981c Compare April 13, 2026 19:35
@MasterPtato MasterPtato force-pushed the 04-12-fix_pegboard-runner_clear_terminal_tunnel_routes branch from 8088fbf to 24f0e66 Compare April 13, 2026 19:35
@github-actions
Copy link
Copy Markdown
Contributor

Preview packages published to npm

Install with:

npm install rivetkit@pr-4621

All packages published as 0.0.0-pr.4621.b2be34f with tag pr-4621.

Engine binary is shipped via @rivetkit/engine-cli on linux-x64-musl, linux-arm64-musl, darwin-x64, and darwin-arm64. Windows users should use the release installer or set RIVET_ENGINE_BINARY.

Docker images:

docker pull rivetdev/engine:slim-b2be34f
docker pull rivetdev/engine:full-b2be34f
Individual packages
npm install rivetkit@pr-4621
npm install @rivetkit/react@pr-4621
npm install @rivetkit/rivetkit-native@pr-4621
npm install @rivetkit/sqlite-wasm@pr-4621
npm install @rivetkit/workflow-engine@pr-4621

@MasterPtato MasterPtato marked this pull request as ready for review April 13, 2026 20:40
Copy link
Copy Markdown
Contributor

MasterPtato commented Apr 13, 2026

Merge activity

  • Apr 13, 8:40 PM UTC: A user started a stack merge that includes this pull request via Graphite.
  • Apr 13, 8:46 PM UTC: Graphite rebased this pull request as part of a merge.
  • Apr 13, 8:47 PM UTC: @MasterPtato merged this pull request with Graphite.

@MasterPtato MasterPtato changed the base branch from 04-12-fix_pegboard_skip_protocol_version_keys_in_runner_pool_backfill to graphite-base/4621 April 13, 2026 20:43
@MasterPtato MasterPtato changed the base branch from graphite-base/4621 to main April 13, 2026 20:44
@MasterPtato MasterPtato force-pushed the 04-12-fix_pegboard-runner_clear_terminal_tunnel_routes branch from 24f0e66 to 3edf76b Compare April 13, 2026 20:45
@MasterPtato MasterPtato merged commit 7c84b2b into main Apr 13, 2026
17 of 21 checks passed
@MasterPtato MasterPtato deleted the 04-12-fix_pegboard-runner_clear_terminal_tunnel_routes branch April 13, 2026 20:47
@NathanFlurry NathanFlurry mentioned this pull request Apr 13, 2026
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants